-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port resteasy fix for sub-resources #42905
Port resteasy fix for sub-resources #42905
Conversation
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...lient/runtime/src/main/java/io/quarkus/restclient/runtime/QuarkusProxyInvocationHandler.java
Outdated
Show resolved
Hide resolved
1b2ca87
to
48d0ee7
Compare
I concur it would be very nice to have a test. You should be able to add one in the |
This comment has been minimized.
This comment has been minimized.
e0f3a31
to
f72a099
Compare
...lient/runtime/src/main/java/io/quarkus/restclient/runtime/QuarkusProxyInvocationHandler.java
Outdated
Show resolved
Hide resolved
@maxr2011-tech11 please try to add a test for this change. The resteasy-microprofile PR you linked also has one, so it should be a matter of mimicking that same scenario here in the Otherwise, it looks OK. |
This comment has been minimized.
This comment has been minimized.
f72a099
to
367a08c
Compare
This comment has been minimized.
This comment has been minimized.
367a08c
to
53410ab
Compare
@manovotn just added the SubResourceTest to the deployment module of the resteasy-classic/resteasy-client extension. also I did rewrite it a bit to use the quarkus integrated rest client inside the internal quarkus junit module instead of the Arquillian extension which resteasy uses (not sure if this works out) CC @geoand |
53410ab
to
cb6f13f
Compare
This comment has been minimized.
This comment has been minimized.
46843b3
to
9df0827
Compare
This comment has been minimized.
This comment has been minimized.
The pipeline checks report, that there are some tests failing, specifically those where a rest client is involved, so it has to be something in the code. No idea what it could be since I am not that deeply into the implementation of the rest client, maybe you could have a look or involve someone who has more insights? CC @stesie |
First of all, the test you added doesn't pass.
From a quick glance, you forgot to register the exception mapper to the rest client builder. This affects two of the tests.
You want to do:
|
@maxr2011-tech11 I've pushed a commit fixing the test since I already had it locally. I also added |
1b7adff
to
733c938
Compare
I have launched CI |
Right, I was just looking into it. Also, running the test locally I am seeing the following logged (which may or may not be due to how the test is written, looking into that):
|
...esteasy-client/deployment/src/test/java/io/quarkus/restclient/exception/SubResourceTest.java
Outdated
Show resolved
Hide resolved
b9ce6d3
to
15f6a36
Compare
Also to answer this one: but is directly injected into here:
Just found out during debugging. So it should work either way. But if you want I can add additional tests that use the QuarkusRestClientBuilder instead of the Annotations (but I don't think that would be necessary, since also the other test classes in general prefer to use the Annotations) |
Yes, I also found that out through debugging.
Since we now know it is used in either approach, you can keep it as is. Assuming the CI passes, I have no further issues with this PR but we should wait for @jamezp to take a look as well. |
This comment has been minimized.
This comment has been minimized.
...esteasy-client/deployment/src/test/java/io/quarkus/restclient/exception/SubResourceTest.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
public static class TestExceptionMapper implements ResponseExceptionMapper<TestException> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a CDI qualifying annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will annotate it as 'Singleton' since it is also registered as @Provider
it should be fine either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no annotation needed here but it doesn't really matter that it is present.
15f6a36
to
7a74094
Compare
7a74094
to
9161d88
Compare
Status for workflow
|
@maxr2011-tech11 thanks for contributing! 👍 |
Port resteasy fix for sub-resources from resteasy/resteasy-microprofile#241